Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow headers to be provided to fetch_url #499

Closed
wants to merge 2 commits into from

Conversation

brendon
Copy link

@brendon brendon commented Dec 2, 2018

Closes #498

Allows optional headers to be passed to the request. The interface for headers is a bit weird so we need to loop the passed in headers and then add each one using a hash-like [] interface. It seems they can't be all added at once.

@brendon
Copy link
Author

brendon commented Aug 26, 2019

Hi @markevans, would you mind looking at this PR? Let me know if you think it needs further improvement.

@brendon
Copy link
Author

brendon commented Aug 21, 2023

Hi @markevans, I'm upgrading the app where I use a fork of dragonfly with this PR applied. I notice you're still actively maintaining the gem so I wondered if you'd consider accepting this PR for inclusion? I'm happy to make any modifications you think necessary.

@brendon
Copy link
Author

brendon commented Aug 30, 2024

Hi @markevans, is this something that we could still look to add?

@markevans
Copy link
Owner

Hi - sorry for the unbelievably slow response on this! 😬

Looks good in principle, however, I don't think we can include this because of the following reason (sorry I didn't think about this before)...

The job steps, including the arguments, get encoded into the url, so for example

job = Dragonfly.app.fetch_url("https://example.com/image")
job.url # "/W1siZnUiLCJodHRwczovL2V4YW1wbGUuY29tL2ltYWdlIl1d/image?sha=033ecdd26d3968ad"
job = Dragonfly.app.fetch_url("https://example.com/image", headers: {"authorization"=> "Bearer abc123"})
job.url # "/W1siZnUiLCJodHRwczovL2V4YW1wbGUuY29tL2ltYWdlIix7ImhlYWRlcnMiOnsiQXV0aG9yaXphdGlvbiI6IkJlYXJlciBhYmMxMjMifX1dXQ/image?sha=4cf8250ff0fa8974"

(note the longer url).

As you can see the url encodes the auth details - this can even be decoded:

Dragonfly::Serializer.json_b64_decode("W1siZnUiLCJodHRwczovL2V4YW1wbGUuY29tL2ltYWdlIix7ImhlYWRlcnMiOnsiQXV0aG9yaXphdGlvbiI6IkJlYXJlciBhYmMxMjMifX1dXQ") # ==> [["fu", "https://example.com/image", {"headers"=>{"Authorization"=>"Bearer abc123"}}]]

For your app a better idea would be to retrieve the image yourself using whichever http library, then if assigning to an activerecord model do it as per https://markevans.github.io/dragonfly/models#using-the-accessors.

To be honest if I were to start again I may not have even included fetch_url - it's probably better to require the user to save an image locally themselves rather than encoding fetch details in the url.

Apologies I didn't really have time to think this through previously

@brendon
Copy link
Author

brendon commented Sep 2, 2024

Hi @markevans, absolutely not a problem! :D I'm a gem maintainer myself and know how real paying works takes priority :)

I didn't realise it worked this way. I assume the fetch only happens once at the time the record is assigned and then Dragonfly stores its own copy of the image? This is my use case (ActiveRecord):

module GoogleDriveConcern

  extend ActiveSupport::Concern

  GOOGLE_DRIVE_FILE_URL = 'https://www.googleapis.com/drive/v3/files'

  included do
    before_validation :set_file_from_google_drive, if: -> { google_drive_file_id.present? }
  end

  private

  def set_file_from_google_drive
    self.file = Dragonfly.app.fetch_url "#{GOOGLE_DRIVE_FILE_URL}/#{google_drive_file_id}?alt=media",
      headers: { 'Authorization': "Bearer #{google_drive_oauth_token}" }
    self.file.name = google_drive_file_name
  end

end

The google_drive_... attributes are just in-memory attributes on the model:

  attribute :google_drive_file_id, :string
  attribute :google_drive_file_name, :string
  attribute :google_drive_oauth_token, :string

Is the concern just that someone with access to the job queue could acquire the token?

As you say though, it's easy enough to fetch the file using Faraday or something like that instead and sidestep all of this.

@markevans
Copy link
Owner

In your case, yes the fetch is only happening when assigned and Dragonfly is storing its own copy (the line self.file = ...) so you don't have to worry about leaking auth details.

But Dragonfly provides extra functionality such that fetch_url can generate its own url, e.g.

url = Dragonfly.app.fetch_url("https://some.url/image").url # --> "/adsfkladfk..."

If you were to visit this url Dragonfly would fetch that url and serve it (you can chain on processing steps etc.).
You're not using this functionality and 99% of people probably won't either, but it needs to be taken into account.

However it's not really in Dragonfly's remit to provide extra HTTP functionality beyond v. basic as this can easily be done with Faraday or something as you mentioned.

I think the best bet would probably be to replace the line

self.file = Dragonfly.app.fetch_url(...

with

self.file = SomeLibraryLikeFaraday.get("...

Hope that makes sense

@brendon
Copy link
Author

brendon commented Sep 3, 2024

Thanks @markevans, that definitely makes sense. I suspected it would be something like that. Thanks for the detailed explainer :) Above and beyond!

All the best :D

@brendon brendon closed this Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding the ability to specify headers for a URL based attachment
2 participants